Skip to content

Conversation

msanatan
Copy link
Contributor

@msanatan msanatan commented Aug 15, 2025

Addresses #214 . This is to get the pipeline going, I only added a very basic test for now. We can add tests for the execution tools bit by bit, and as bugs and the like come up.

I'm using Unity 2021 in the project since that's the lowest version we support.

Here's it working in my repo: https://github.com/msanatan/unity-mcp/actions/runs/16981560211

Summary by CodeRabbit

  • Tests

    • Added a Unity test project with Edit Mode tests for command handling.
    • Configured test assemblies and package dependencies for reliable execution.
  • Chores

    • Introduced an automated GitHub Actions workflow to run Unity tests on pushes.
    • Added Unity project settings and a comprehensive .gitignore to streamline development.
    • Included a sample scene and basic script for validation within the test project.

@msanatan msanatan requested a review from dsarno August 15, 2025 02:46
@msanatan msanatan self-assigned this Aug 15, 2025
Copy link
Contributor

coderabbitai bot commented Aug 15, 2025

Walkthrough

Adds a Unity edit mode test project (UnityMCPTests) with packages, settings, sample script, and tests; introduces assembly definition for tests; creates Unity meta files; adds a Unity-focused .gitignore; and sets up a GitHub Actions workflow to run Unity tests via game-ci on pushes to main.

Changes

Cohort / File(s) Summary
CI Workflow
.github/workflows/unity-tests.yml
New GitHub Actions workflow to run Unity editmode tests using game-ci, with cache, secrets for license, and artifact upload.
Project Scaffolding & Settings
TestProjects/UnityMCPTests/.gitignore, .../ProjectSettings/ProjectVersion.txt, .../ProjectSettings/Packages/com.unity.testtools.codecoverage/Settings.json
Adds Unity-specific .gitignore; sets Unity editor version; adds code coverage package settings.
Packages
TestProjects/UnityMCPTests/Packages/manifest.json, .../Packages/packages-lock.json
Defines UPM dependencies including local com.coplaydev.unity-mcp and locks all package versions and sources.
Assets: Scenes & Scripts
TestProjects/UnityMCPTests/Assets/Scenes.meta, .../Assets/Scenes/SampleScene.unity.meta, .../Assets/Scripts.meta, .../Assets/Scripts/Hello.cs, .../Assets/Scripts/Hello.cs.meta
Adds scene and script folders/meta; introduces a simple MonoBehaviour Hello logging “Hello World”.
Tests (EditMode)
TestProjects/UnityMCPTests/Assets/Tests.meta, .../Assets/Tests/EditMode.meta, .../Assets/Tests/EditMode/CommandRegistryTests.cs, .../Assets/Tests/EditMode/CommandRegistryTests.cs.meta, .../Assets/Tests/EditMode/UnityMCPTests.EditMode.asmdef, .../UnityMCPTests.EditMode.asmdef.meta
Adds EditMode test assembly definition and NUnit tests for CommandRegistry.GetHandler, with accompanying Unity meta files.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor Dev as Developer
  participant GH as GitHub
  participant GA as GitHub Actions
  participant Unity as game-ci/unity-test-runner

  Dev->>GH: Push to main (affected paths)
  GH-->>GA: Trigger "Unity Tests" workflow
  GA->>GA: Checkout repo (LFS)
  GA->>GA: Restore Library cache
  GA->>Unity: Run editmode tests (2021.3.45f1)
  Unity-->>GA: Test results (artifactsPath)
  GA->>GH: Upload test results artifact (always)
Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

I thump my paws—tests hop into play,
A scene, a script, a whiskered “Hooray!”
CI clouds churn, green lights bloom,
NUnit nibble checks dispel the gloom.
With unity pips and artifact treats,
This bunny approves—continuous feats! 🐇✨

Tip

🔌 Remote MCP (Model Context Protocol) integration is now available!

Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats.

✨ Finishing Touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

CodeRabbit Commands (Invoked using PR/Issue comments)

Type @coderabbitai help to get the list of available commands.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Status, Documentation and Community

  • Visit our Status Page to check the current availability of CodeRabbit.
  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (14)
TestProjects/UnityMCPTests/Assets/Scripts/Hello.cs (1)

7-9: Prefer explicit access modifier on Unity message methods

Minor clarity improvement: make Start explicitly private.

-    void Start()
+    private void Start()

Additionally, consider wrapping this sample script in a namespace to avoid global type collisions in the test project (change outside this hunk shown below):

using UnityEngine;

namespace UnityMCPTests.Scripts
{
    public class Hello : MonoBehaviour
    {
        private void Start()
        {
            Debug.Log("Hello World");
        }
    }
}
TestProjects/UnityMCPTests/Packages/manifest.json (2)

6-8: Trim IDE integration packages from the test project to speed up resolution.

These IDE packages aren’t needed in CI or for running tests and slow down dependency resolution.

Apply this diff:

-    "com.unity.ide.rider": "3.0.31",
-    "com.unity.ide.visualstudio": "2.0.22",
-    "com.unity.ide.vscode": "1.2.5",

2-46: Avoid relying on transitive Newtonsoft.Json; declare it explicitly.

Tests/imports reference Newtonsoft.Json. Today it arrives transitively via the bridge package, but that’s brittle. Declare the dependency explicitly to keep the test project self-sufficient.

Apply this diff to add the package:

     "com.unity.test-framework": "1.1.33",
+    "com.unity.nuget.newtonsoft-json": "3.0.2",
     "com.unity.textmeshpro": "3.0.6",
TestProjects/UnityMCPTests/Assets/Tests/EditMode/UnityMCPTests.EditMode.asmdef (1)

14-18: Prefer default references over manual overrides; drop precompiledReferences.

Overriding references makes the asm brittle and requires manually listing NUnit/Json DLLs. Let Unity’s Test Framework provide NUnit; add Newtonsoft via manifest (see earlier comment) instead of a DLL name here.

Apply this diff:

-  "overrideReferences": true,
-  "precompiledReferences": [
-    "nunit.framework.dll",
-    "Newtonsoft.Json.dll"
-  ],
+  "overrideReferences": false,
+  "precompiledReferences": [],

If you plan to keep overrideReferences true, at least remove Newtonsoft here after adding the package in manifest, because Unity will resolve it automatically:

   "precompiledReferences": [
-    "nunit.framework.dll",
-    "Newtonsoft.Json.dll"
+    "nunit.framework.dll"
   ],
TestProjects/UnityMCPTests/.gitignore (1)

79-82: Ignore test result outputs as well.

Unity Test Runner and CI workflows can produce local TestResults folders/files. Consider ignoring them to avoid accidental commits.

Apply this diff:

 # TestRunner generated files
 InitTestScene*.unity*
 
+# Unity Test Runner results (local runs)
+/[Tt]est[Rr]esults*/
+/[Tt]est[Rr]esults.*
TestProjects/UnityMCPTests/Assets/Tests/EditMode/CommandRegistryTests.cs (2)

2-2: Remove unused using directive.

using Newtonsoft.Json; isn’t used in this test file.

Apply this diff:

-using Newtonsoft.Json;

24-27: Harden the assertions: use nameof and assert static.

Using nameof reduces string literal fragility, and checking IsStatic makes the intent explicit.

Apply this diff:

-            var methodInfo = handler.Method;
-            Assert.AreEqual("HandleCommand", methodInfo.Name, "Handler method name should be HandleCommand.");
+            var methodInfo = handler.Method;
+            Assert.AreEqual(nameof(ManageGameObject.HandleCommand), methodInfo.Name, "Handler method name should be HandleCommand.");
             Assert.AreEqual(typeof(ManageGameObject), methodInfo.DeclaringType, "Handler should be declared on ManageGameObject.");
-            Assert.IsNull(handler.Target, "Handler should be a static method (no target instance).");
+            Assert.IsTrue(methodInfo.IsStatic, "Handler should be a static method.");
+            Assert.IsNull(handler.Target, "Static method delegates must have null Target.");
.github/workflows/unity-tests.yml (5)

3-10: Consider running tests on pull_request and broadening path filters.

Right now tests run only on pushes to main and won’t gate PRs. Also, runtime changes under UnityMcpBridge outside Editor won’t trigger the workflow.

Apply this diff to include PRs and trigger on all UnityMcpBridge changes:

 on:
   push:
     branches: [ main ]
     paths:
       - TestProjects/UnityMCPTests/**
-      - UnityMcpBridge/Editor/**
+      - UnityMcpBridge/**
       - .github/workflows/unity-tests.yml
+  pull_request:
+    branches: [ main ]
+    paths:
+      - TestProjects/UnityMCPTests/**
+      - UnityMcpBridge/**
+      - .github/workflows/unity-tests.yml

31-38: Strengthen cache key to avoid stale Library caches on package changes.

The current key omits dependency manifests, risking stale cache reuse after package updates.

Include OS and hash of manifest.json and packages-lock.json in the key (with robust fallbacks):

-      - uses: actions/cache@v4
+      - uses: actions/cache@v4
         with:
-          path: ${{ matrix.projectPath }}/Library
-          key: Library-${{ matrix.projectPath }}-${{ matrix.unityVersion }}
+          path: ${{ matrix.projectPath }}/Library
+          key: Library-${{ runner.os }}-${{ matrix.projectPath }}-${{ matrix.unityVersion }}-${{ hashFiles(format('{0}/Packages/manifest.json', matrix.projectPath), format('{0}/Packages/packages-lock.json', matrix.projectPath)) }}
           restore-keys: |
-            Library-${{ matrix.projectPath }}-
-            Library-
+            Library-${{ runner.os }}-${{ matrix.projectPath }}-${{ matrix.unityVersion }}-
+            Library-${{ runner.os }}-${{ matrix.projectPath }}-
+            Library-${{ runner.os }}-
+            Library-

41-52: Optional: Enable code coverage in test runs.

You already include com.unity.testtools.codecoverage; enable it to publish coverage in CI.

       - name: Run tests
         uses: game-ci/unity-test-runner@v4
         id: tests
         env:
           UNITY_EMAIL: ${{ secrets.UNITY_EMAIL }}
           UNITY_PASSWORD: ${{ secrets.UNITY_PASSWORD }}
           UNITY_LICENSE: ${{ secrets.UNITY_LICENSE }}
         with:
           projectPath: ${{ matrix.projectPath }}
           unityVersion: ${{ matrix.unityVersion }}
           testMode: ${{ matrix.testMode }}
+          customParameters: -enableCodeCoverage -coverageResultsPath ./Coverage -coverageOptions generateAdditionalMetrics;generateHtmlReport

Follow-up: Update the artifact upload (below) to include the coverage directory if desired.


12-23: Minor: Job name suggests multiple modes but matrix has only editmode.

Either rename the job or add playmode when ready. No change needed now if intentional.

Example adjustment:

-  testAllModes:
-    name: Test in ${{ matrix.testMode }}
+  unity-tests:
+    name: Unity ${{ matrix.testMode }} tests

Or add playmode later:

         testMode:
           - editmode
+          - playmode

1-24: Optional hardening: Add concurrency cancelation and timeout to prevent stuck jobs and CI backlog.

 name: Unity Tests

 on:
   push:
@@
   testAllModes:
     name: Test in ${{ matrix.testMode }}
     runs-on: ubuntu-latest
+    timeout-minutes: 60

And at workflow root (top-level), consider:

+concurrency:
+  group: unity-tests-${{ github.ref }}
+  cancel-in-progress: true
TestProjects/UnityMCPTests/Packages/packages-lock.json (2)

71-79: Git-sourced IDE package in CI increases variability (com.unity.ide.windsurf).

A git dependency for IDE integration isn’t needed for headless CI and can slow/harden reproducibility. It also pulls a test-framework dependency (1.1.9) that’s older than your resolved 1.1.33.

Consider removing com.unity.ide.windsurf from Packages/manifest.json in the test project to keep CI lean.
If desired, I can propose a manifest.json patch.


32-45: Feature bundle ‘com.unity.feature.development’ may be overkill for CI.

This pulls multiple IDE and profiling tools not required for running tests, increasing restore time.

For the CI test project, consider depending directly on com.unity.test-framework and com.unity.testtools.codecoverage only, and remove the feature bundle from the test project’s manifest.json.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 60a2ed4 and 16fb151.

⛔ Files ignored due to path filters (1)
  • TestProjects/UnityMCPTests/Assets/Scenes/SampleScene.unity is excluded by !**/*.unity
📒 Files selected for processing (17)
  • .github/workflows/unity-tests.yml (1 hunks)
  • TestProjects/UnityMCPTests/.gitignore (1 hunks)
  • TestProjects/UnityMCPTests/Assets/Scenes.meta (1 hunks)
  • TestProjects/UnityMCPTests/Assets/Scenes/SampleScene.unity.meta (1 hunks)
  • TestProjects/UnityMCPTests/Assets/Scripts.meta (1 hunks)
  • TestProjects/UnityMCPTests/Assets/Scripts/Hello.cs (1 hunks)
  • TestProjects/UnityMCPTests/Assets/Scripts/Hello.cs.meta (1 hunks)
  • TestProjects/UnityMCPTests/Assets/Tests.meta (1 hunks)
  • TestProjects/UnityMCPTests/Assets/Tests/EditMode.meta (1 hunks)
  • TestProjects/UnityMCPTests/Assets/Tests/EditMode/CommandRegistryTests.cs (1 hunks)
  • TestProjects/UnityMCPTests/Assets/Tests/EditMode/CommandRegistryTests.cs.meta (1 hunks)
  • TestProjects/UnityMCPTests/Assets/Tests/EditMode/UnityMCPTests.EditMode.asmdef (1 hunks)
  • TestProjects/UnityMCPTests/Assets/Tests/EditMode/UnityMCPTests.EditMode.asmdef.meta (1 hunks)
  • TestProjects/UnityMCPTests/Packages/manifest.json (1 hunks)
  • TestProjects/UnityMCPTests/Packages/packages-lock.json (1 hunks)
  • TestProjects/UnityMCPTests/ProjectSettings/Packages/com.unity.testtools.codecoverage/Settings.json (1 hunks)
  • TestProjects/UnityMCPTests/ProjectSettings/ProjectVersion.txt (1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/CommandRegistryTests.cs (2)
UnityMcpBridge/Editor/Tools/CommandRegistry.cs (1)
  • CommandRegistry (10-45)
UnityMcpBridge/Editor/Tools/ManageGameObject.cs (1)
  • ManageGameObject (20-2153)
TestProjects/UnityMCPTests/Assets/Scripts/Hello.cs (1)
UnityMcpBridge/Editor/Tools/ManageScript.cs (1)
  • System (759-825)
🔇 Additional comments (18)
TestProjects/UnityMCPTests/Assets/Scenes/SampleScene.unity.meta (1)

1-7: Unity .meta file is correct and GUIDs are unique

DefaultImporter block is standard and minimal, and tracking .meta files is required for stable GUID references.
Verified across the project that there are zero duplicate GUIDs in any .meta files—everything looks good.

TestProjects/UnityMCPTests/Assets/Scripts.meta (1)

1-8: Folder .meta is fine

Correctly marked as folderAsset with default importer fields empty. No issues.

TestProjects/UnityMCPTests/ProjectSettings/ProjectVersion.txt (1)

1-2: CI unityVersion aligned with ProjectVersion
The GitHub Actions workflow pins unityVersion to 2021.3.45f1 in the strategy matrix and uses that same value in the test step, matching ProjectVersion.txt. No further changes required.

TestProjects/UnityMCPTests/Assets/Tests/EditMode.meta (1)

1-8: EditMode asmdef is correctly restricted to Editor and references the expected assemblies

  • Location: TestProjects/UnityMCPTests/Assets/Tests/EditMode/UnityMCPTests.EditMode.asmdef
  • includePlatforms: only "Editor"
  • references: "UnityMcpBridge.Editor", "UnityEngine.TestRunner", "UnityEditor.TestRunner"
  • defineConstraints: "UNITY_INCLUDE_TESTS"

No changes required.

TestProjects/UnityMCPTests/Assets/Scenes.meta (1)

1-8: LGTM: Standard Unity folder meta

This looks like a proper folder asset meta with a stable GUID. Good to keep these committed for deterministic imports.

TestProjects/UnityMCPTests/Assets/Tests.meta (1)

1-8: LGTM: Tests folder meta committed

Consistent with Unity’s defaults; having this GUID tracked avoids churn across machines.

TestProjects/UnityMCPTests/Assets/Tests/EditMode/UnityMCPTests.EditMode.asmdef.meta (1)

1-7: Remove direct overrides and precompiled references from the EditMode asmdef

Your EditMode assembly already has the correct includePlatforms and Unity Test Runner references. However, it’s still overriding references and pulling in direct NUnit/JSON binaries. To rely on the Unity Test Framework package instead, remove the overrideReferences line (or set it to false) and delete the precompiledReferences block.

• File:
TestProjects/UnityMCPTests/Assets/Tests/EditMode/UnityMCPTests.EditMode.asmdef
• Changes needed:

  • Remove or set "overrideReferences" to false.
  • Remove the "precompiledReferences" array.

Suggested diff:

--- a/TestProjects/UnityMCPTests/Assets/Tests/EditMode/UnityMCPTests.EditMode.asmdef
+++ b/TestProjects/UnityMCPTests/Assets/Tests/EditMode/UnityMCPTests.EditMode.asmdef
@@
-  "overrideReferences": true,
-  "precompiledReferences": [
-    "nunit.framework.dll",
-    "Newtonsoft.Json.dll"
-  ],

Likely an incorrect or invalid review comment.

TestProjects/UnityMCPTests/Assets/Scripts/Hello.cs.meta (1)

1-11: LGTM: Script meta looks standard

Standard MonoImporter settings with a stable GUID. All good.

TestProjects/UnityMCPTests/Packages/manifest.json (1)

10-10: Test Framework version looks correct for 2021 LTS.

com.unity.test-framework@1.1.33 aligns with Unity 2021.3.x. Good choice.

TestProjects/UnityMCPTests/Assets/Tests/EditMode/UnityMCPTests.EditMode.asmdef (2)

5-8: References are scoped appropriately for EditMode tests.

Referencing UnityMcpBridge.Editor and the TestRunner assemblies is correct for running editor-only tests.


9-11: Editor platform restriction is correct.

Limiting this test assembly to Editor avoids accidental inclusion in player builds.

TestProjects/UnityMCPTests/.gitignore (1)

1-106: Solid Unity .gitignore for a nested project.

Covers the usual Unity-generated folders, IDE caches, and test artifacts. Good placement at the project root.

TestProjects/UnityMCPTests/Assets/Tests/EditMode/CommandRegistryTests.cs (2)

10-16: LGTM: unknown command returns null.

This test asserts the expected null for an unmapped command.


18-28: Clarify case-sensitivity expectations and consider adding a test.

CommandRegistry’s code snippet comment suggests case-insensitive lookup, but the implementation appears case-sensitive. Decide on the desired behavior and codify it in tests.

If case-insensitive behavior is desired, one more test like this would help:

[Test]
public void GetHandler_IsCaseInsensitive_WhenConfigured()
{
    var handler = CommandRegistry.GetHandler("handlemanagegameobject");
    // If intended to be case-insensitive, this should be non-null.
    // If case-sensitive is intentional, assert IsNull instead.
    Assert.IsNotNull(handler, "Expected case-insensitive lookup for command names.");
}

If case-sensitive behavior is intentional, update comments in CommandRegistry to match and consider adding a negative case-sensitivity test asserting null for mismatched casing.

TestProjects/UnityMCPTests/Assets/Tests/EditMode/CommandRegistryTests.cs.meta (1)

1-11: Meta file looks fine.

GUID and MonoImporter settings are standard for a C# script meta.

.github/workflows/unity-tests.yml (1)

45-47: Verify secrets exist in the repository or organization.

This workflow requires UNITY_EMAIL, UNITY_PASSWORD, UNITY_LICENSE. If missing or mis-scoped (e.g., set at org but not allowed on PRs from forks), jobs will fail.

If you want, I can provide a small preflight step to emit a clear error when secrets are absent.

TestProjects/UnityMCPTests/Packages/packages-lock.json (2)

3-10: Newtonsoft.Json version mismatch (declared 3.0.2 vs resolved 3.2.1).

Your local package declares com.unity.nuget.newtonsoft-json 3.0.2, but the lock resolves 3.2.1. Likely fine, but if UnityMcpBridge uses APIs pinned to 3.0.2, this could bite.

Recommendation: In UnityMcpBridge/package.json, relax to a range (e.g., ^3.0.2) or bump to 3.2.1 to align with the lock. Example:

{
  "name": "com.coplaydev.unity-mcp",
  "version": "0.0.0",
  "dependencies": {
    "com.unity.nuget.newtonsoft-json": "^[3.0.2]"
  }
}

If you prefer an exact pin, update both the package and lock to 3.2.1.

Also applies to: 80-86


1-416: Lock file looks consistent with Unity 2021.3 LTS and test infra.

Resolved versions and builtin modules align with the targeted editor version and the Unity Test Framework.

"com.unity.ide.rider": "3.0.31",
"com.unity.ide.visualstudio": "2.0.22",
"com.unity.ide.vscode": "1.2.5",
"com.unity.ide.windsurf": "https://github.com/Asuta/com.unity.ide.windsurf.git",
Copy link
Contributor

@coderabbitai coderabbitai bot Aug 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Verification agent

🧩 Analysis chain

Unpinned git dependency poses supply-chain and reproducibility risk; remove or pin.

com.unity.ide.windsurf is sourced from a git URL without a commit/tag. This can change between runs, break CI, or introduce malicious code. It also brings no value to headless CI test runs.

Apply this diff to remove it:

-    "com.unity.ide.windsurf": "https://github.com/Asuta/com.unity.ide.windsurf.git",

If you truly need it, pin to a specific commit:

-    "com.unity.ide.windsurf": "https://github.com/Asuta/com.unity.ide.windsurf.git",
+    "com.unity.ide.windsurf": "https://github.com/Asuta/com.unity.ide.windsurf.git#<commit-sha>"

Run this to confirm nothing in the repo references Windsurf, so it’s safe to remove:


🏁 Script executed:

#!/bin/bash
rg -n -C2 -i 'windsurf' || true

Length of output: 6530


Remove unpinned Windsurf dependency from test project manifest

The com.unity.ide.windsurf entry in TestProjects/UnityMCPTests/Packages/manifest.json is a floating git URL with no tag or commit SHA. This poses a supply-chain and reproducibility risk and isn’t exercised by any tests in this project. Please:

• In TestProjects/UnityMCPTests/Packages/manifest.json, remove the Windsurf line.
• In TestProjects/UnityMCPTests/Packages/packages-lock.json, remove the corresponding com.unity.ide.windsurf block.

Example diff:

--- TestProjects/UnityMCPTests/Packages/manifest.json
@@ -6,4 +6,3 @@
     "com.unity.ide.vscode": "1.2.5",
-    "com.unity.ide.windsurf": "https://github.com/Asuta/com.unity.ide.windsurf.git",
     "com.unity.test-framework": "1.1.33",
     "com.unity.textmeshpro": "3.0.6"
--- TestProjects/UnityMCPTests/Packages/packages-lock.json
@@ -69,7 +69,4 @@
       "source": "git",
       "revision": "HEAD"
     },
-    "com.unity.ide.windsurf": {
-      "version": "https://github.com/Asuta/com.unity.ide.windsurf.git",
-      "depth": 0,
-      "source": "git"
-    },
     "com.unity.test-framework": {
       "version": "1.1.33",
       "depth": 1,

If you still need Windsurf in the test project, pin it to a specific commit:

- "com.unity.ide.windsurf": "https://github.com/Asuta/com.unity.ide.windsurf.git",
+ "com.unity.ide.windsurf": "https://github.com/Asuta/com.unity.ide.windsurf.git#<commit-sha>"
🤖 Prompt for AI Agents
In TestProjects/UnityMCPTests/Packages/manifest.json around line 9, remove the
"com.unity.ide.windsurf": "https://github.com/Asuta/com.unity.ide.windsurf.git",
entry to eliminate the unpinned floating git dependency; additionally open
TestProjects/UnityMCPTests/Packages/packages-lock.json and remove the entire
com.unity.ide.windsurf block so the lockfile no longer references the package
(or if Windsurf is required, replace the manifest entry with a git URL pinned to
a specific tag or commit SHA and update the packages-lock.json accordingly).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's used by the developer, should be fine but good points. Will look for a better package later

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@msanatan Understood! You're right that for local development it's fine. When you do get a chance to address it later, the main benefit will be more reliable CI runs since pinned dependencies eliminate the "works on my machine but fails in CI" scenarios. Thanks for considering the feedback!

(\   /)

( ..)
o
(")(")

Comment on lines +1 to +7
{
"m_Name": "Settings",
"m_Path": "ProjectSettings/Packages/com.unity.testtools.codecoverage/Settings.json",
"m_Dictionary": {
"m_DictionaryValues": []
}
} No newline at end of file
Copy link
Contributor

@coderabbitai coderabbitai bot Aug 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Verification agent

🧩 Analysis chain

Code coverage settings file is valid; confirm CI enables coverage flags

The settings JSON is fine. To actually collect coverage in CI, ensure the runner passes the code coverage arguments (e.g., -enableCodeCoverage) and that the com.unity.testtools.codecoverage package is present for 2021 LTS.

Run:


🏁 Script executed:

#!/bin/bash
# Verify manifest contains the code coverage package and check CI args
echo "Manifest dependencies (looking for com.unity.testtools.codecoverage):"
rg -n '"com.unity.testtools.codecoverage"\s*:\s*' TestProjects/UnityMCPTests/Packages/manifest.json -n -C1 || true

echo
echo "Workflow coverage flags (-enableCodeCoverage / codeCoverage):"
fd -H 'unity-tests.yml' .github | while read -r f; do
  echo "==> $f"
  rg -n -C2 'enableCodeCoverage|codeCoverage|-coverage|coverageOptions' "$f" || true
done

Length of output: 659


Enable Unity code coverage in CI and include the coverage package

The Settings.json looks good, but no coverage package is referenced and the workflow isn’t passing any coverage flags. To fix:

  • Add the code-coverage package to your project manifest
    ­File: TestProjects/UnityMCPTests/Packages/manifest.json
    Under "dependencies", insert:
    "com.unity.testtools.codecoverage": "<latest-compatible-version>"

  • Enable coverage flags in your GitHub Actions workflow
    ­File: .github/workflows/unity-tests.yml
    In the Unity Test Runner step, either:
    • Set the action input enableCodeCoverage: true
    • Or add -enableCodeCoverage (and any other required -coverage CLI args) to the arguments block

Verify you’re using the correct package version for Unity 2021 LTS and that the CI job publishes the resulting coverage report.

🤖 Prompt for AI Agents
In
TestProjects/UnityMCPTests/ProjectSettings/Packages/com.unity.testtools.codecoverage/Settings.json
lines 1-7, add the code coverage package to
TestProjects/UnityMCPTests/Packages/manifest.json by inserting
"com.unity.testtools.codecoverage": "<latest-compatible-version>" under
"dependencies" (pick the version compatible with Unity 2021 LTS), and update
.github/workflows/unity-tests.yml so the Unity Test Runner step enables coverage
by setting the action input enableCodeCoverage: true or by adding the CLI args
-enableCodeCoverage and any required -coverage flags to the arguments block;
also ensure the CI job is configured to publish the generated coverage report.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Coverage is something we'll think of later

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@msanatan Understood! You're absolutely right to focus on getting the basic test infrastructure working first. Code coverage can definitely be added in a future PR once the testing foundation is solid. The current setup with the basic unit tests is a great starting point.

🐰💭
(Thoughtful about project priorities)

@dsarno
Copy link
Collaborator

dsarno commented Aug 15, 2025

Will look tomorrow!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The actual test, just added a basic one. Planning to add more bit by bit in separate PRs

Copy link
Collaborator

@dsarno dsarno left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@msanatan Fantastic, this is a great start. I pulled it and ran it, even wrote a few new tests of some of the tools, maybe I'll add those after this is merged. Nice that it's all set up as a sibling folder outside of the package too, and the autorun / CI will be great.

Main comment: because these are only Unity tests, we're missing key parts of the MCP cycle, which are on my mind since that's what I've been working on the past week, including the client connection stuff from earlier this week, and the "protocol framing" robustness in my upcoming PR. What's missing:

  • MCP handshake / lifecycle (initialize, ping/heartbeat).
  • Tool schema validation (the server advertises manage_gameobject with an input schema; clients validate/shape args from that).
  • JSON‑RPC framing & error codes (e.g., invalid params vs. tool-level errors).
  • Transport quirks (stdio buffering/ordering, streamable HTTP we you use it).
  • Server routing (mapping tools/call → manage_gameobject.HandleCommand) and result content formatting (MCP content parts).

So we really should add a true E2E testing suite (client → server → Unity → server → client)
The goal would be to start Unity (with the Bridge), start the Python MCP server, then call tools/call over stdio using the official SDK. That exercises the full pipeline defined by the MCP spec, and then we catch the whole rainbow of possible regressions.

With that said, good to go from my end!

@msanatan
Copy link
Contributor Author

Thanks @dsarno! I definitely want to get a start because I want to add more tools and features haha. However, we're definitely on the same page with regards to testing goals! I think this set up will kick off the end-to-end testing that'll put us in a good place.

I'll merge in, cheers

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants